r? @ehuss

(rust-highfive has picked a reviewer for you, use r? to override)

Oops. Seems some url like file:///path/#bar is valid in some scenarios. Should we enforce the "must have a host" rule or having a "fallback to a empty host" is enough?

Thanks for taking a look at this. Unfortunately, specs are kind of a mess right now (see #7725). The cargo: protocol probably needs to just be removed. Once you do that, I think it should error correctly when given a bare path.

It might be nice to detect the scenario someone gives a path and provide a better error message (like maybe suggest using file://).

Updated. However it still unwraps a None if someone passes cargo:///path without a host deliberately. Should this case be handle by a empty host as a fallback?

So, I was thinking it could flip the :// check around, maybe something like this:

if spec.contains("://") {
    if let Ok(url) = spec.into_url() {
        return PackageIdSpec::from_url(url);
    }
} else if spec.contains('/') || spec.contains('\\') {
    let abs = std::env::current_dir().unwrap_or_default().join(spec);
    if abs.exists() {
        let maybe_url = Url::from_file_path(abs)
            .map_or_else(|_| "a file:// URL".to_string(), |url| url.to_string());
        bail!(
            "package ID specification `{}` looks like a file path, \
            maybe try {}",
            spec,
            maybe_url
        );
    }
}

And completely remove the cargo:// stuff, including the check for cargo in Display. The check for startswith / will only catch absolute paths on Unix, which I think is a little too narrow.

The error I wrote above still isn't great. Ideally it would actually tell you what the correct value is. query_str is capable of doing that, so if you are interested in trying to provide an accurate error message, that could be nice, though that would be a fair bit more work.

The tests need to be updated, and ideally there would be a UI test (maybe just add another case to suggestion_bad_pkgid).

Also, the documentation should be updated (cargo-pkgid.md, then rebuild the man pages).

@ehuss
All you suggestions are patched!
Not sure what part of document should be refined, so I only add an example for querying local package's pkgid.

Thanks!

Sorry, I think I was mistaken on the docs, but your change seems fine. I was thinking of the pkgid-spec.md file. I'll follow up with an update to that.

@bors r+

@ehuss: 🔑 Insufficient privileges: Not in reviewers

@bors r+

📌 Commit c5d2304 has been approved by ehuss

⌛ Testing commit c5d2304 with merge 0f3f47e...

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 0f3f47e to master...

Posted #9249 with an update to the spec docs that I was thinking of.